Skip to content

refactor(executor): move inline unit tests to src/tests/ module#619

Merged
MauroToscano merged 5 commits into
mainfrom
cleanup/executor-inline-tests
May 26, 2026
Merged

refactor(executor): move inline unit tests to src/tests/ module#619
MauroToscano merged 5 commits into
mainfrom
cleanup/executor-inline-tests

Conversation

@diegokingston
Copy link
Copy Markdown
Collaborator

@diegokingston diegokingston commented May 22, 2026

Summary

Relocates the three inline #[cfg(test)] mod tests blocks in executor/src into a dedicated executor/src/tests/ module, matching the convention already used by the prover and math crates.

From To Covers
flamegraph.rs tests/flamegraph_tests.rs demangle
vm/instruction/execution.rs tests/keccak_tests.rs keccak-f[1600] + Keccak syscall
vm/memory.rs tests/memory_tests.rs public-output commits, load bounds

demangle and Memory::set_bytes_aligned were module-private; bumped to pub(crate) so the relocated tests can reach them. No test logic changed — bodies moved verbatim, only use super::* rewritten to explicit crate:: paths.

Test plan

  • cargo clippy -p executor --all-targets -- -D warnings — clean
  • cargo test -p executor --lib — 12 passed (1 demangle + 4 keccak + 7 memory)

@diegokingston
refactor(executor): move inline unit tests to src/tests/ module
f888c8d 
Relocates the three inline `#[cfg(test)] mod tests` blocks in executor/src
into a dedicated `executor/src/tests/` module, matching the convention
already used by the prover and math crates.

- flamegraph.rs                -> tests/flamegraph_tests.rs (demangle)
- vm/instruction/execution.rs  -> tests/keccak_tests.rs (keccak-f[1600] + syscall)
- vm/memory.rs                 -> tests/memory_tests.rs (public output, load bounds)

`demangle` and `Memory::set_bytes_aligned` were module-private; bumped to
pub(crate) so the relocated tests can reach them. No test logic changed.
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 22, 2026

Codex Code Review

No security vulnerabilities, behavioral bugs, or significant performance issues found in the PR diff.

One minor cleanliness note: executor/src/lib.rs now exports pub mod tests; in normal builds, but the submodules are #[cfg(test)], so it becomes an empty public executor::tests module outside tests. That is likely harmless, but #[cfg(test)] mod tests; would avoid exposing test-only API surface.

I attempted cargo test -p executor --no-run, but the environment failed before compiling because rustup could not create a temp file under the read-only /home/runner/.rustup/tmp.

claude[bot]
claude Bot reviewed May 22, 2026
View reviewed changes
Comment thread executor/src/lib.rs
claude[bot]
claude Bot reviewed May 22, 2026
View reviewed changes
Comment thread executor/src/vm/memory.rs
@claude
Copy link
Copy Markdown

claude Bot commented May 22, 2026

Review

Clean refactor overall — test bodies are moved verbatim and the logic is unchanged. One bug to fix before merging, plus a minor nit.

Bug — missing #[cfg(test)] on the tests module (Medium)

executor/src/lib.rs declares pub mod tests; without a #[cfg(test)] gate. The two crates cited as the convention to match (prover, math) both use:

#[cfg(test)]
pub mod tests;

Without the gate the tests module ships into every non-test build and becomes part of the crate's public API (executor::tests). It will be empty in those builds (all sub-modules in mod.rs are individually gated), but it still pollutes the public surface and diverges from the stated goal.

Nit — pub(crate) on set_bytes_aligned (Low)

The function's own doc comment says it "Should only be used to write to public output and private input." pub(crate) makes it reachable from anywhere in the crate, widening the accidental-misuse surface. Worth keeping it narrower if possible (e.g. pub(super) or a test-only re-export).

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 22, 2026

Benchmark Results for unmodified programs 🚀

Command Mean [ms] Min [ms] Max [ms] Relative
base binary_search 50.6 ± 2.5 48.8 57.4 1.00
head binary_search 52.0 ± 5.2 48.6 65.3 1.03 ± 0.12
Command Mean [ms] Min [ms] Max [ms] Relative
base bitwise_ops 50.1 ± 0.8 48.7 50.9 1.00 ± 0.03
head bitwise_ops 50.0 ± 1.4 48.5 52.2 1.00
Command Mean [ms] Min [ms] Max [ms] Relative
base fibonacci_26 55.2 ± 0.9 54.1 56.5 1.00
head fibonacci_26 56.5 ± 3.2 53.9 64.2 1.02 ± 0.06
Command Mean [ms] Min [ms] Max [ms] Relative
base hashmap 106.5 ± 1.7 104.6 109.5 1.00
head hashmap 107.7 ± 3.0 103.5 114.8 1.01 ± 0.03
Command Mean [ms] Min [ms] Max [ms] Relative
base keccak 108.5 ± 0.7 107.5 109.6 1.00 ± 0.02
head keccak 108.4 ± 1.7 105.6 110.7 1.00
Command Mean [ms] Min [ms] Max [ms] Relative
base matrix_multiply 61.9 ± 1.1 60.4 63.9 1.01 ± 0.03
head matrix_multiply 61.6 ± 1.6 59.9 65.4 1.00
Command Mean [ms] Min [ms] Max [ms] Relative
base modular_exp 50.5 ± 2.1 49.0 56.0 1.00
head modular_exp 50.6 ± 1.0 49.1 51.8 1.00 ± 0.05
Command Mean [ms] Min [ms] Max [ms] Relative
base quicksort 53.2 ± 0.7 51.7 54.0 1.01 ± 0.02
head quicksort 52.5 ± 0.8 51.6 53.9 1.00
Command Mean [ms] Min [ms] Max [ms] Relative
base sieve 55.9 ± 1.1 54.7 57.6 1.00
head sieve 55.9 ± 1.1 54.6 57.4 1.00 ± 0.03
Command Mean [ms] Min [ms] Max [ms] Relative
base sum_array 66.5 ± 1.0 65.0 67.9 1.00
head sum_array 67.0 ± 1.2 65.7 68.8 1.01 ± 0.02
Command Mean [ms] Min [ms] Max [ms] Relative
base syscall_commit 78.6 ± 2.8 76.0 85.2 1.00
head syscall_commit 82.8 ± 9.4 75.4 102.4 1.05 ± 0.12

@MauroToscano MauroToscano enabled auto-merge May 26, 2026 17:21
@MauroToscano MauroToscano added this pull request to the merge queue May 26, 2026
Merged via the queue into main with commit 222a7fa May 26, 2026
18 checks passed
@MauroToscano MauroToscano deleted the cleanup/executor-inline-tests branch May 26, 2026 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jotabulacios jotabulacios jotabulacios approved these changes

@claude claude[bot] claude[bot] left review comments

@MauroToscano MauroToscano MauroToscano approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@diegokingston @MauroToscano @jotabulacios